fix: argocd version --client tries to connect server #8341

gdsoumya · 2022-02-02T05:05:31Z

Signed-off-by: Soumya Ghosh Dastidar gdsoumya@gmail.com

All commands call the PreRun hook and try to startup the local API server even when it's not required leading to the errors noticed. The proposed solution is to move the local API server creation from the PreRun hook into the argocdclient.NewClientOrDie call which will make sure the server is created only when a client needs to access it.

#Fixes #7986

Checklist:

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

codecov · 2022-02-02T05:17:13Z

Codecov Report

Merging #8341 (2db0076) into master (b009cdb) will increase coverage by 0.40%.
The diff coverage is 3.00%.

@@            Coverage Diff             @@
##           master    #8341      +/-   ##
==========================================
+ Coverage   41.91%   42.31%   +0.40%     
==========================================
  Files         178      176       -2     
  Lines       23003    22797     -206     
==========================================
+ Hits         9641     9646       +5     
+ Misses      11979    11775     -204     
+ Partials     1383     1376       -7

Impacted Files	Coverage Δ
cmd/argocd/commands/account.go	`0.00% <0.00%> (ø)`
cmd/argocd/commands/admin/app.go	`30.50% <0.00%> (ø)`
cmd/argocd/commands/admin/dashboard.go	`0.00% <0.00%> (ø)`
cmd/argocd/commands/app.go	`0.53% <0.00%> (-0.01%)`	⬇️
cmd/argocd/commands/app_actions.go	`0.00% <0.00%> (ø)`
cmd/argocd/commands/cert.go	`0.00% <0.00%> (ø)`
cmd/argocd/commands/cluster.go	`5.68% <0.00%> (ø)`
cmd/argocd/commands/gpg.go	`0.00% <0.00%> (ø)`
cmd/argocd/commands/login.go	`2.30% <0.00%> (ø)`
cmd/argocd/commands/project.go	`0.00% <0.00%> (ø)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b009cdb...2db0076. Read the comment docs.

jessesuen · 2022-02-02T05:25:37Z

Based on #7986 (comment), the issue turns out to be a bit deeper. I think we need to improve the CLI in general such that the assumption of a valid kube context is only used when necessary. For example, is it possible to fix this such that argocd app doesn't fail with an invalid kube context?

jessesuen · 2022-02-02T05:31:44Z

I feel like the logic for starting startInProcessAPI should be moved out of the PersistentPreRunE that sets up the CLI commands. @alexmt can you think of a better place to conditionally start this?

EDIT: Nevermind I see this is conditionally added to commands that need to access the API server:

	command.AddCommand(NewCompletionCommand())
	command.AddCommand(headless.InitCommand(NewVersionCmd(&clientOpts), &clientOpts, nil, nil))
	command.AddCommand(headless.InitCommand(NewClusterCommand(&clientOpts, pathOpts), &clientOpts, nil, nil))
	command.AddCommand(headless.InitCommand(NewApplicationCommand(&clientOpts), &clientOpts, nil, nil))
	command.AddCommand(NewLoginCommand(&clientOpts))
	command.AddCommand(NewReloginCommand(&clientOpts))
	command.AddCommand(headless.InitCommand(NewRepoCommand(&clientOpts), &clientOpts, nil, nil))
	command.AddCommand(headless.InitCommand(NewRepoCredsCommand(&clientOpts), &clientOpts, nil, nil))
	command.AddCommand(NewContextCommand(&clientOpts))
	command.AddCommand(headless.InitCommand(NewProjectCommand(&clientOpts), &clientOpts, nil, nil))
	command.AddCommand(headless.InitCommand(NewAccountCommand(&clientOpts), &clientOpts, nil, nil))
	command.AddCommand(NewLogoutCommand(&clientOpts))
	command.AddCommand(headless.InitCommand(NewCertCommand(&clientOpts), &clientOpts, nil, nil))
	command.AddCommand(headless.InitCommand(NewGPGCommand(&clientOpts), &clientOpts, nil, nil))
	command.AddCommand(admin.NewAdminCommand())

jessesuen · 2022-02-02T05:52:45Z

I think I've come to the conclusion the only way to fix this for argocd app, argocd repo, etc... is to remove the call to headless.InitCommand outside of root.go and instead move them inside the app, repo, etc... for each subcommand. Similar to

argo-cd/cmd/argocd/commands/admin/dashboard.go

Line 28 in 8b57bc9

headless.InitCommand(cmd, clientOpts, &port, &address)

@alexmt any better ideas?

jessesuen · 2022-02-02T06:32:03Z

Actually, I think the cleaner approach would be to create a wrapper around argocdclient.NewClientOrDie(clientOpts) which starts up the throwaway local API server depending on if clientOpts.Core is set.

gdsoumya · 2022-02-02T06:35:55Z

For commands like the version --client, it doesn't have any subcommands and probably still needs hardcoded checks in InitCommand.

Also I took a look at how cobra parses the command and for commands with subcommands, it appears that len(cmd.Commands())>0 (subcommand count) if that command is called without calling a subcommand. On the other hand, if the subcommand is called cmd actually points to the subcommand and not the parent command which means that len(cmd.Command()) points to the no. of commands that are children of the current subcommand.

With this if we can make an assumption that all top-level commands will only perform actions when a subcommand is called and only display the HelpFunc() when called alone (which is the case for all the commands as far as I can see) we could put a check in the PreRun hook:

if len(cmd.Commands())>0{
  return nil
  }

This would skip the hook execution if a top-level command is called without the subcommands and will display the help data.

gdsoumya · 2022-02-02T06:37:14Z

This is a more generalized solution and will work for any depth of subcommands but if there are subcommands without any more children commands that need to skip this hook that would need to an extra check like the version command.

gdsoumya · 2022-02-02T06:41:05Z

Actually, I think the cleaner approach would be to create a wrapper around argocdclient.NewClientOrDie(clientOpts) which starts up the throwaway local API server depending on if clientOpts.Core is set.

This is actually better as it only executes when a call to the server is needed.

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

gdsoumya · 2022-02-02T11:06:53Z

@jessesuen have updated the PR according to your suggestion, let me know if this is good.

jessesuen

Could you group the imports such that we have:

standard libs in one block
3rd party libs in 2nd block
github.com/argoproj/argo-cd in the third block

cmd/argocd/commands/initialize/cmd.go

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

Signed-off-by: Soumya Ghosh Dastidar <soumya@akuity.io>

jessesuen · 2022-02-05T00:48:11Z

cmd/argocd/commands/account.go

@@ -72,7 +74,7 @@ has appropriate RBAC permissions to change other accounts.
 				c.HelpFunc()(c, args)
 				os.Exit(1)
 			}
-			acdClient := argocdclient.NewClientOrDie(clientOpts)
+			acdClient := headless.NewClientOrDie(clientOpts, initialize.RetrieveContextIfChanged(c.Flag("context")))


I think we can do this a little bit cleaner. Everywhere we repeat a call to initialize.RetrieveContextIfChanged when initializing the client. i.e.:

headless.NewClientOrDie(clientOpts, initialize.RetrieveContextIfChanged(c.Flag("context")))

Can we somehow improve/reduce this to:

headless.NewClientOrDie(clientOpts)

Can we update the clientOpts to have a .context field so we don't have to parse this everywhere?

I was initially thinking of doing that but this specific context is only required when we need to create the local server and the CLientOptions struct is a more general client pkg data that might be used by other API clients and binaries so I felt it might be better to keep it specific to the cli only instead of modifying the original struct. But we can add it in there too.

I see your point. But unfortunately, there's already a clientOpts.Core which is a useless flag to the generic argocd client package. So for better or worse, we already have CLI specific flag which made its way into in clientOpts.

If we want to be slightly cleaner about this, we could preserve the existing API client constructor. e.g.:

apiclient.NewClientOrDie(clientOpts)

And introduce a CLI specific constructor that accepts the cobra.Command as a second argument:

headless.NewClientOrDie(clientOpts, c)

The headless.NewClientOrDie would call:

initialize.RetrieveContextIfChanged(c.Flag("context")) ... apiclient.NewClientOrDie(clientOpts)

Then we don't have to introduce more useless options to clientOpts which aren't really used by the API client package. And we could even remove the Core option from clientOpts. WDYT?

That sounds good to me, also we'll shift the headless.NewClientOrDie to the CLI pkg so it will not technically change anything in the apiclient pkg.

jessesuen · 2022-02-05T00:49:53Z

pkg/apiclient/headless/headless.go

+	"github.com/alicebob/miniredis/v2"
+	"github.com/go-redis/redis/v8"
+	"github.com/golang/protobuf/ptypes/empty"
+	log "github.com/sirupsen/logrus"
+	"github.com/spf13/pflag"
+	"k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/client-go/kubernetes"
+	cache2 "k8s.io/client-go/tools/cache"
+	"k8s.io/client-go/tools/clientcmd"
+	"k8s.io/utils/pointer"
+
+	"github.com/argoproj/argo-cd/v2/pkg/apiclient"
+	"github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
+	appclientset "github.com/argoproj/argo-cd/v2/pkg/client/clientset/versioned"
+	repoapiclient "github.com/argoproj/argo-cd/v2/reposerver/apiclient"
+	"github.com/argoproj/argo-cd/v2/server"
+	servercache "github.com/argoproj/argo-cd/v2/server/cache"
+	"github.com/argoproj/argo-cd/v2/util/cache"
+	appstatecache "github.com/argoproj/argo-cd/v2/util/cache/appstate"
+	"github.com/argoproj/argo-cd/v2/util/cli"
+	"github.com/argoproj/argo-cd/v2/util/io"
+	kubeutil "github.com/argoproj/argo-cd/v2/util/kube"
+	"github.com/argoproj/argo-cd/v2/util/localconfig"


I think we might be bringing in too many dependencies into pkg/apiclient, which is meant to be a library/SDK. I think this package still belongs next to CLI code rather than library code.

If we are okay with shifting the NewClientOrDie() to the CLI (which should be okay because it's only used there) we can move this pkg to the cli part. NewClientOrDie needs to access the headless methods which if kept in the apiClient pkg separate from headless in cli, causes import cycles.

If we are okay with shifting the NewClientOrDie() to the CLI (which should be okay because it's only used there) we can move this pkg to the cli part.

Spoke with @alexmt and he agrees that we should move this logic into CLI specific package to keep dependencies of pkg/apiclient leaner.

Will shift it to CLI

Signed-off-by: Soumya Ghosh Dastidar <soumya@akuity.io>

jessesuen

This is much cleaner approach. Thank you!

jessesuen · 2022-02-10T01:38:13Z

pkg/apiclient/apiclient.go

-// NewClientOrDie creates a new API client from a set of config options, or fails fatally if the new client creation fails.
-func NewClientOrDie(opts *ClientOptions) Client {
-	client, err := NewClient(opts)
-	if err != nil {
-		log.Fatal(err)
-	}
-	return client
-}
-


I know we don't use this anymore in the CLI, but since it exists in pkg i think we may want to keep this for other projects importing argo-cd as a library.

Yes exactly, that's why I did not remove this incase someone else is already using it.

Wait according to this diff. You did remove this.

Oh sorry, I thought I moved the code around in the same pkg. Let me add it back.

Reverted it back.

Signed-off-by: Soumya Ghosh Dastidar <soumya@akuity.io>

fixed issue with version --client cmd

6c957f0

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

gdsoumya mentioned this pull request Feb 2, 2022

argocd version --client does not work offline in --core mode #7986

Closed

emretanriverdi approved these changes Feb 2, 2022

View reviewed changes

gdsoumya added 3 commits February 2, 2022 16:13

moved local api server creation to NewClientOrDie()

3ee3065

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

lint fixes

873ee06

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

revert makefile change

e36bf7a

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

jessesuen reviewed Feb 3, 2022

View reviewed changes

cmd/argocd/commands/initialize/cmd.go Show resolved Hide resolved

gdsoumya and others added 4 commits February 3, 2022 11:48

fixed go imports order

7d61c55

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

revert makefile change

49b01b5

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

lint fix

f5a717b

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

reverted generated file changes

8977ba2

Signed-off-by: Soumya Ghosh Dastidar <soumya@akuity.io>

jessesuen requested changes Feb 5, 2022

View reviewed changes

fix: moved headless to cli

62937ca

Signed-off-by: Soumya Ghosh Dastidar <soumya@akuity.io>

jessesuen approved these changes Feb 10, 2022

View reviewed changes

alexmt approved these changes Feb 10, 2022

View reviewed changes

refactor: reverted back original NewCLientOrDie

2db0076

Signed-off-by: Soumya Ghosh Dastidar <soumya@akuity.io>

jessesuen merged commit c078bbb into argoproj:master Feb 10, 2022

joebowbeer mentioned this pull request Feb 10, 2022

'argocd version --client' fails if no valid Kubernetes context after using 'argocd login --core' #7426

Open

3 tasks

danielhelfand mentioned this pull request Mar 31, 2022

argocd version --client tries to reach server #8931

Closed

3 tasks

joebowbeer mentioned this pull request Jun 9, 2022

Core mode should allow persistent usage of custom K8s client configuration #7992

Open

danielhelfand mentioned this pull request Jun 27, 2022

argocd version prints incorrect server version #9747

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: argocd version --client tries to connect server #8341

fix: argocd version --client tries to connect server #8341

gdsoumya commented Feb 2, 2022 •

edited

Loading

codecov bot commented Feb 2, 2022 •

edited

Loading

jessesuen commented Feb 2, 2022

jessesuen commented Feb 2, 2022 •

edited

Loading

jessesuen commented Feb 2, 2022

jessesuen commented Feb 2, 2022

gdsoumya commented Feb 2, 2022

gdsoumya commented Feb 2, 2022

gdsoumya commented Feb 2, 2022

gdsoumya commented Feb 2, 2022

jessesuen left a comment

jessesuen Feb 5, 2022

gdsoumya Feb 5, 2022

jessesuen Feb 9, 2022

gdsoumya Feb 9, 2022

gdsoumya Feb 9, 2022

jessesuen Feb 5, 2022

gdsoumya Feb 5, 2022

jessesuen Feb 9, 2022

gdsoumya Feb 9, 2022

gdsoumya Feb 9, 2022

jessesuen left a comment

jessesuen Feb 10, 2022

gdsoumya Feb 10, 2022

jessesuen Feb 10, 2022

gdsoumya Feb 10, 2022

gdsoumya Feb 10, 2022

fix: argocd version --client tries to connect server #8341

fix: argocd version --client tries to connect server #8341

Conversation

gdsoumya commented Feb 2, 2022 • edited Loading

codecov bot commented Feb 2, 2022 • edited Loading

Codecov Report

jessesuen commented Feb 2, 2022

jessesuen commented Feb 2, 2022 • edited Loading

jessesuen commented Feb 2, 2022

jessesuen commented Feb 2, 2022

gdsoumya commented Feb 2, 2022

gdsoumya commented Feb 2, 2022

gdsoumya commented Feb 2, 2022

gdsoumya commented Feb 2, 2022

jessesuen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessesuen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdsoumya commented Feb 2, 2022 •

edited

Loading

codecov bot commented Feb 2, 2022 •

edited

Loading

jessesuen commented Feb 2, 2022 •

edited

Loading